[EIM-371] [Test] Add proxy isolation for GUI offline installer #285
Conversation
Add block-list feature to proxy server
Summary of ChangesHello @Fabricio-ESP, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure by introducing a new "block-list" feature to the existing proxy server. This allows for more precise control over network access during automated tests, enabling scenarios where specific domains or all internet connections need to be blocked. The primary application of this feature is to ensure the robustness of the EIM GUI's offline installer by isolating its network environment during tests. The changes involve updates across test runner classes, the proxy utility, and test suite configurations to integrate and leverage this new capability effectively. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the testing framework by adding proxy isolation for GUI tests and introducing a new 'block-list' mode to the test proxy server. This allows for more granular control over network access during tests, which is a great improvement for testing offline scenarios and mirror configurations.
The refactoring moves environment setup logic into the TestProxy class, which is a good centralization of responsibility. However, I've found a critical issue in CLITestRunner.class.js where it uses a stale environment, preventing the proxy settings from being applied to CLI tests. I've also suggested an improvement in TestProxy.class.js to avoid replacing the process.env object, which is a safer approach.
Overall, the changes are valuable for improving test coverage and reliability. Please address the critical issue to ensure the proxy isolation works as intended for all tests.
| fullArgs = this.args, | ||
| isolatedEnvironment = false, | ||
| } = {}) { | ||
| async start({ command = this.command, fullArgs = this.args } = {}) { |
There was a problem hiding this comment.
This refactoring has introduced a critical issue. The CLITestRunner instance is created before the proxy is started in the test scripts. The CLITestRunner constructor caches process.env into this.environment. Later, TestProxy.start() modifies process.env, but the CLITestRunner instance still holds the old, stale environment. Consequently, when testRunner.start() is called, it spawns a process using the stale environment, and the proxy settings are not applied.
To fix this, CLITestRunner should get the latest environment when spawning the process. A possible fix is to change the pty.spawn call inside the start method to use process.env directly instead of this.environment.
| setEnvironment() { | ||
| logger.info("Setting proxy environment variables"); | ||
| const proxyUrl = "http://127.0.0.1:8888"; | ||
| let environment = { ...process.env }; | ||
| environment.HTTP_PROXY = proxyUrl; | ||
| environment.HTTPS_PROXY = proxyUrl; | ||
| environment.http_proxy = proxyUrl; | ||
| environment.https_proxy = proxyUrl; | ||
| environment.FTP_PROXY = proxyUrl; | ||
| environment.ftp_proxy = proxyUrl; | ||
| environment.NO_PROXY = "127.0.0.1,localhost,::1"; | ||
| environment.no_proxy = "127.0.0.1,localhost,::1"; | ||
| environment.CARGO_HTTP_PROXY = proxyUrl; | ||
| environment.CARGO_HTTPS_PROXY = proxyUrl; | ||
| environment.GIT_PROXY_COMMAND = ""; | ||
| environment.npm_config_proxy = proxyUrl; | ||
| environment.npm_config_https_proxy = proxyUrl; | ||
| environment.PIP_PROXY = proxyUrl; | ||
| process.env = { ...environment }; | ||
| } |
There was a problem hiding this comment.
Replacing the process.env object is generally not recommended and can be unsafe as it is a deprecated practice in Node.js. It's better to modify the properties of the process.env object directly. This avoids replacing the object that other parts of the application might have a reference to.
setEnvironment() {
logger.info("Setting proxy environment variables");
const proxyUrl = "http://127.0.0.1:8888";
process.env.HTTP_PROXY = proxyUrl;
process.env.HTTPS_PROXY = proxyUrl;
process.env.http_proxy = proxyUrl;
process.env.https_proxy = proxyUrl;
process.env.FTP_PROXY = proxyUrl;
process.env.ftp_proxy = proxyUrl;
process.env.NO_PROXY = "127.0.0.1,localhost,::1";
process.env.no_proxy = "127.0.0.1,localhost,::1";
process.env.CARGO_HTTP_PROXY = proxyUrl;
process.env.CARGO_HTTPS_PROXY = proxyUrl;
process.env.GIT_PROXY_COMMAND = "";
process.env.npm_config_proxy = proxyUrl;
process.env.npm_config_https_proxy = proxyUrl;
process.env.PIP_PROXY = proxyUrl;
}
Fabricio-ESP
force-pushed
the
EIM-371-test-add_proxy_isolation_GUI
branch
from
84425b7 to
560f5f6
Compare
Add block-list feature to proxy server
Description
This PR updates the proxy test class in order to allow operation on the EIM GUI.
The EIM GUI is called by selenium webdriver along with the tauri-driver wrapper. Setting up a proxy server on the selenium caller allows to isolate the application running in that environment.
With this feature it is possible to monitor and block any internet connection attempt, validating the operation of the EIM application for offline installation.
The GUI tests have been updated to add proxy isolation based on the test settings, and enabling block mode for the offline installation test.
This PR also adds a new proxy operation mode "block-list" which allows blocking specific domains, for a more granular testing of the network connections stablished by the application.
With this mode, the alternative mirrors test now implements a block to the github.com mirrors.
Related
Testing
Checklist
Before submitting a Pull Request, please ensure the following: